arrow-select: fix MutableArrayData interleave for ListView#9560
arrow-select: fix MutableArrayData interleave for ListView#9560asubiotto wants to merge 1 commit intoapache:mainfrom
Conversation
asubiotto
force-pushed
the
asubiotto/lvmut
branch
from
d049805 to
093008f
Compare
|
I think I prefer #9562 |
|
Happy to go with whichever, my goal is to just fix this for MutableArrayData so we can compare the specialized interleave implementation fairly |
|
Haha that was precisely my comment on #9558 |
The previous code did not extend child data buffers. Signed-off-by: Alfonso Subiotto Marques <alfonso.subiotto@polarsignals.com>
asubiotto
force-pushed
the
asubiotto/lvmut
branch
from
093008f to
80a3d7d
Compare
😆 I think I actually prefer this one as the behavior is "less surprising" to me let me know what you think |
alamb
left a comment
alamb
left a comment
There was a problem hiding this comment.
Thanks @asubiotto (and @vegarsti and @brancz )
compared to #9558, for me this PR is easier to understand and more consistent with the rest of the codebase. Other than the potential offset overflow I think it looks good from my perspective.
Let me know what you think
| } | ||
| mutable.buffer1.push(T::usize_as(new_offset)); | ||
| mutable.buffer2.push(sizes[i]); | ||
| new_offset += size; |
There was a problem hiding this comment.
I think we should be using checked arithmetic here, similarly to
Otherwise the offset could overflow when extending the list
4 participants
The previous code did not extend child data buffers.
I'm preparing a PR for an optimized listview interleave, but wanted to make sure the fallback path was correct before comparing benchmarks.
Which issue does this PR close?
Rationale for this change
Fix a bug
What changes are included in this PR?
Bugfix and test
Are these changes tested?
Yes
Are there any user-facing changes?
ListView interleaves did not succeed previously.